Skip to content

fix(otlptrace,otlpmetric): remove endpoint URL path cleaning #6710

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Jun 3, 2025
Merged

fix(otlptrace,otlpmetric): remove endpoint URL path cleaning #6710

merged 7 commits into from
Jun 3, 2025

Conversation

mjq
Copy link
Contributor

@mjq mjq commented Apr 29, 2025

When setting an explicit OTLP traces endpoint URL (via otlptracehttp.WithEndpointURL or the OTEL_EXPORTER_OTLP_TRACES_ENDPOINT environment variable), any given trailing slash is stripped. This makes it impossible to export traces to an endpoint requiring a trailing slash. It also conflicts with the spec:

For the per-signal variables (OTEL_EXPORTER_OTLP_<signal>_ENDPOINT), the URL MUST be used as-is without any modification. The only exception is that if an URL contains no path part, the root path / MUST be used (see Example 2).

This stripping happens due to the use of path.Clean in otlpconfig.cleanPath. From the path.Clean docs:

The returned path ends in a slash only if it is the root "/".

Other signals appear to use path.Clean and might therefore have the same bug, but I've only tested and fixed traces.

Fixes #6709.

@mjq mjq changed the title Ffix(otlptracehttp): keep traces endpoint URL trailing slashes fix(otlptracehttp): keep traces endpoint URL trailing slashes Apr 29, 2025
@dmathieu
Copy link
Member

the URL MUST be used as-is without any modification

This kind of indicates we should remove path.Clean entirely.

@pellared
Copy link
Member

@mjq, ping 😉

See: #6710 (comment)

@mjq mjq force-pushed the mjq-fix-traces-trailing-slash branch from b07ccf5 to bb3fc07 Compare May 31, 2025 03:44
@mjq
Copy link
Contributor Author

mjq commented May 31, 2025

@pellared Sorry for the delay! I incorporated @dmathieu's feedback re: removing the usage of path.Clean. I removed it from the equivalent config option in metrics as well for consistency.

I also realized I was changing generated files in the original commit - now the changes are made to the templates and applied with make generate.

@mjq mjq changed the title fix(otlptracehttp): keep traces endpoint URL trailing slashes fix(otlptrace,otlpmetric): remove endpoint URL path cleaning May 31, 2025
@pellared
Copy link
Member

pellared commented Jun 1, 2025

What about otlplog?

@codecov Codecov
Copy link

codecov bot commented Jun 1, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.3%. Comparing base (274e939) to head (7c0d68a).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #6710   +/-   ##
=====================================
  Coverage   82.3%   82.3%           
=====================================
  Files        263     263           
  Lines      24421   24421           
=====================================
+ Hits       20099   20102    +3     
+ Misses      3940    3937    -3     
  Partials     382     382
Files with missing lines Coverage Δ
...tlpmetric/otlpmetricgrpc/internal/oconf/options.go 89.0% <100.0%> (ø)
...tlpmetric/otlpmetrichttp/internal/oconf/options.go 91.9% <100.0%> (ø)
...trace/otlptracegrpc/internal/otlpconfig/options.go 94.3% <100.0%> (ø)
...trace/otlptracehttp/internal/otlpconfig/options.go 91.1% <100.0%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mjq mjq force-pushed the mjq-fix-traces-trailing-slash branch from c017642 to 8d9f666 Compare June 2, 2025 02:38
@mjq
Copy link
Contributor Author

mjq commented Jun 2, 2025

I fixed the failing changelog check (bad automatic merge with main).

What about otlplog?

@pellared With this change path.Clean no longer appears in the codebase, and from poking around I couldn't find any equivalent code for log configs.

pellared
pellared approved these changes Jun 2, 2025
View reviewed changes
@pellared
Copy link
Member

pellared commented Jun 2, 2025 •
edited
Loading

I fixed the failing changelog check (bad automatic merge with main).

What about otlplog?

@pellared With this change path.Clean no longer appears in the codebase, and from poking around I couldn't find any equivalent code for log configs.

Thanks. I noticed that otlplog does not even do strings.TrimSpace which is probably OK 😉

@mjq @pellared
Update CHANGELOG.md
a801c1f 
Co-authored-by: Robert Pająk <pellared@hotmail.com>
@mjq
Copy link
Contributor Author

mjq commented Jun 3, 2025

@pellared @dmathieu I don't have permission to merge this, so please go ahead if everything looks good or let me know if there are any further changes I should make 👍 Thanks!

@pellared pellared merged commit 1636bcd into open-telemetry:main Jun 3, 2025
31 of 32 checks passed
@pellared
Copy link
Member

pellared commented Jun 3, 2025

@mjq, thank you very much for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmathieu dmathieu dmathieu approved these changes

@pellared pellared pellared approved these changes

@MrAlias MrAlias Awaiting requested review from MrAlias MrAlias is a code owner

@XSAM XSAM Awaiting requested review from XSAM XSAM is a code owner

@dashpole dashpole Awaiting requested review from dashpole dashpole is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Trailing slashes are stripped from trace endpoint URLs
3 participants
@mjq @dmathieu @pellared